Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JAV-319 Support for Protocol 7 #341

Merged
merged 10 commits into from
Sep 2, 2024
Merged

JAV-319 Support for Protocol 7 #341

merged 10 commits into from
Sep 2, 2024

Conversation

Radiokot
Copy link
Member

Purpose

Support Protocol 7.

Changes

  • Added ProtocolVersion.V7 corresponding to Protocol version 7
  • Added cooldowns list to AccountInfo
  • Added availableBalance to AccountInfo
  • Made optional the following BakerPoolStatus fields:
    bakerEquityCapital, delegatedCapital, delegatedCapitalCap, poolInfo
  • Updated version to 7.3.0-SNAPSHOT

Checklist

  • My code follows the style of this project.
  • The code compiles without warnings.
  • I have performed a self-review of the changes.
  • I have documented my code, in particular the intent of the
    hard-to-understand areas.
  • (If necessary) I have updated the CHANGELOG.

@Radiokot Radiokot requested a review from soerenbf August 29, 2024 09:39
Copy link

@soerenbf soerenbf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO looks good. You need to handle the newly added transaction events however.

Have a look at the added transaction events here: BakerEvent.DelegationRemoved and DelegationEvent.BakerRemoved

@Radiokot
Copy link
Member Author

It seems that the SDK doesn't handle any events, they are only found in gRPC generated classes.

@soerenbf
Copy link

soerenbf commented Aug 30, 2024

It seems that the SDK doesn't handle any events, they are only found in gRPC generated classes.

I don't understand why I cannot find the added types anywhere then? I guess the generated code is not part of the repository?

To me it looks like anything in https://github.com/Concordium/concordium-java-sdk/tree/JAV-319-protocol-7/concordium-sdk/src/main/java/com/concordium/sdk/responses contains classes which are created from the generated GRPC types, and this folder https://github.com/Concordium/concordium-java-sdk/tree/JAV-319-protocol-7/concordium-sdk/src/main/java/com/concordium/sdk/responses/transactionstatus is supposed to (I assume) have 2 extra classes for the respective events linked in my original review:

Have a look at the added transaction events here: BakerEvent.DelegationRemoved and DelegationEvent.BakerRemoved

And of course these would have to be mapped as well somewhere.. If the events are not used it seems like a lot of wasted effort was put into making the already existing conversions?

@Radiokot
Copy link
Member Author

I don't understand why I cannot find the added types anywhere then? I guess the generated code is not part of the repository?

Yes, they get generated locally:
image

Classes under the responses package are manually written human-friendly mappings of GRPC classes involved in ClientV2 calls. The conversion between GRPC and responses is done in ClientV2MapperExtensions.

I'll take a look at transactionstatus.

Breaking changes in BakerPoolStatus
Also remove bakerId from AbstractBakerResult.
Also remove delegatorId from AbstractDelegatorResult.
@Radiokot Radiokot requested a review from soerenbf August 30, 2024 09:29
Copy link

@soerenbf soerenbf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some minor inconsistencies. When they're fixed, feel free to merge 😄

@Radiokot Radiokot merged commit 3032c9f into main Sep 2, 2024
2 checks passed
@Radiokot Radiokot deleted the JAV-319-protocol-7 branch September 2, 2024 11:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants